-
Notifications
You must be signed in to change notification settings - Fork 34
[WIP] Separate server from protocol and application #95
Conversation
e437068
to
f3ffeab
Compare
f462b40
to
2848dcc
Compare
5750300
to
fe72b65
Compare
Ok the "protocol" part is ready for review, there is a unit-test for that. I haven't run the actual server yet, or otherwise tested it, so that part would still require additional work. |
fn check_response(&self, response: &Response) -> Result<(), ()> { | ||
let has_expected = match self.response_type { | ||
ResponseType::Info => response.has_info(), | ||
ResponseType::SetOption => response.has_set_option(), | ||
ResponseType::Query => response.has_query(), | ||
ResponseType::CheckTx => response.has_check_tx(), | ||
ResponseType::InitChain => response.has_init_chain(), | ||
ResponseType::BeginBlock => response.has_begin_block(), | ||
ResponseType::DeliverTx => response.has_deliver_tx(), | ||
ResponseType::EndBlock => response.has_end_block(), | ||
ResponseType::Commit => response.has_commit(), | ||
ResponseType::Flush => response.has_flush(), | ||
ResponseType::Echo => response.has_echo(), | ||
ResponseType::Exception => response.has_exception(), | ||
}; | ||
if !has_expected { | ||
return Err(()); | ||
} | ||
Ok(()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking whether this dynamic checking could be turned more into static checking (at least enforce it for the client / implementor) -- e.g. if there's Responder<T: ResponseType> ...
and the methods in Application
would then require to be instantiated with the correct type, something like
fn deliver_tx(&mut self, _p: RequestDeliverTx, responder: Responder<ResponseDeliverTx>) {....
Something like this pattern: https://yoric.github.io/post/rust-typestate/
@gterzian any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is an interesting idea, and I think it would require making a bunch of other stuff generic as well, for example
- I believe that if the
Application
starts having generic methods, you can'tBox
it anymore. There are ways around that, by makingProtocol
itself generic. - Unless we make
Response
itself generic, the check is going to have to be at runtime I think. And if we makeResponse
generic, it will require making all sorts of other things generic as well.
So instead of making the current Protocol
generic,
I think one might want to try to spawn one Protocol
, or rather Session
per "session", and type the whole thing based on the back and forth messaging you expect using for example those session types.
You would probably still need one "master" Protocol
, running like the current one, to handle other stuff (like query/echo and other potential "out of protocol" requests?) and to spawn typed "child" Session<T>
or something similar.
It might also require re-thinking the Application
trait, and perhaps supplementing it with some AppSession<T>
or similar, that would represent the handling of a single block and fully-type the protocol.
Probably best done in a potential follow-up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current proposal looks good -- my main concern in this proposal is that it's a little bit less type-safe than the previous one -- e.g. as a client / implementor, I may by mistake pass to deliver_tx
a Responder
that returns ResponseCheckTx
(instead of ResponseDeliverTx
). Ideally, this would be caught at compile-time rather than runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if I understand what you mean, you're concern is that the variant of request/response is a runtime thing, not compile-time.
In the current version, each method of Application
has a specific Request_*
as argument and returns a specific Response_*
.
This version has a single Responder
, that then does some sanity check at runtime using an enum of response types.
Loosing some "type-safety" will happen each time you use an enum
as type. You can catch errors at compile-time, at the cost of of having to use a struct
for each variant, versus a single enum
.
I could do the same by introducing a ResponderCheckTx
, ResponderDeliverTx
, Responder_*
, and having the protocol select on a dozen channel, one for each "type".
That might be even better than using a generic approach, however you can't say it's pretty or concise code. It probably leaves plenty of room for logical errors anyway(the more insidious ones probably).
So I think it might be a bad trade-off, you get fairly minimal "type-safety" preventing one class of programming errors at compile vs runtime, and in return you get convoluted code.
FInally, in terms of programming error. Code that compile doesn't necessarily run as you'd expect. You can still make a mistake and define a method as using DeliverTx
where CheckTx
belongs, no one might notice, and the app author just follows the compiler. That wouldn't ensure correct runtime behavior either.
let res = ResponseDeliverTx::new(); | ||
let mut response = Response::new(); | ||
response.set_deliver_tx(res); | ||
let _ = responder.respond(response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other thought -- why throw away Result<(), ()>
here and not return them?
if it's returned, https://github.com/tendermint/rust-abci/pull/95/files#diff-b4aea3e418ccdb71239b96952d9cddb6R329 could crash immediately (so there are no silent errors)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean make it the return value of the trait method? In that case it would have to be a sync call, while now the app can pass the responder to other threads and eventually call respond
on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some general feedback:
- Especially when you factor in the threading and futures handling, the code can be hard to follow. I spent a good 20-30 minutes trying to figure out what calls what, and the opaqueness of the public API may make it hard for developers to figure out internal errors, or how the framework is actually executing and processing their code. It would be nice to put a little text diagram in the comments, or somewhere, that would show how
Server
,Protocol
, andApplication
send messages back and forth between each-other. Also, my experience with Tokio is limited, but shouldn'tProtocol
act more like a future that is executed on the Tokio runtime, instead of being spawned on a system thread? - Why does the counter example need a separate
CounterProxy
? Couldn't you just implementApplication
directly onCounterApp
? Why do you need the added overhead of channels?CounterApp
is repeatedly polling a channel on a separate thread, but since that's all it's doing, it doesn't make sense for it to be running separately. Just execute the functions directly.
Besides that, I just have problems with the concept. In my mind, Application
represents the state machine that the application is simulating. In other words, it should be updating or checking the state, and then returning a response. To me, it doesn't make a huge amount of sense why the public API should be concerned with sending this data over a oneshot channel. It would make more sense for it just to return the response and have the handler for the Application
, which is Protocol
, send that over a channel. As a matter of fact, why use channels at all? Protocol
is just a message passer, an unneeded layer of indirection between the server and application. Not to say that I'm against everything in this PR: the start
method could be potentially useful for an application, if it passed resources from the server. But this PR seems to be adding complication for the sake of having information being passed over channels. I don't see the need for all these channels to be in use, especially when we're only running one Application at a time, maximum.
TL;DR: This PR, in essence, is too complicated.
impl CounterProxy { | ||
fn new() -> Self { | ||
let (sender, receiver) = unbounded(); | ||
CounterProxy { | ||
sender, | ||
receiver: Some(receiver), | ||
exiter: None, | ||
} | ||
} | ||
} | ||
|
||
impl CounterProxy { | ||
fn exit_abci(&self) { | ||
let _ = self | ||
.exiter | ||
.as_ref() | ||
.expect("Proxy to have an exiter") | ||
.exit(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably could be combined under a single impl
block.
let mut response = Response::new(); | ||
let res = ResponseInfo::new(); | ||
response.set_info(res); | ||
let _ = responder.respond(response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite a lot of boilerplate code! ResponseInfo::new()
is much more readable then this 4-line sequence that only produces the default response. Also, info
returns ()
, but I think it would make more sense to return a Result
of some kind, so any errors could be propagated. Right now, Protocol::handle_network_request
makes the assumption that no internal errors occur when processing a request, but that isn't necessarily true. Either we should pass errors through the respective response methods, Protocol::handle_network_request
and Protocol::run
, or Responder::respond
shouldn't return Result<(), ()>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A single call to ResponseInfo::new()
is indeed simpler, and it's also semantically different so I don't think a comparison is in order.
As to error handling, it might make sense for the application to send some errors to the protocol via an additional method on Responder
. Alternatively, it could also use to Exiter
to stop the protocol and server, if there is no way to recover from the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know the semantics are different, I was just pointing out how this change makes things a lot more verbose.
It might be better if the responder wrapped the specific response type in a Response
before sending it through the channel, so we could do this:
responder.respond_info(ResponseInfo::new())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could be another way to achieve it, and there you'd end-up with a lot more methods on the Responder
.
You could perhaps solve that problem by introducing a AppResponse
enum, pass that to respond
, which would match on it and internally create the response and call the appropriate set_
method on it with the inner value of the enum variant.
The public API is not meant to reflect the internals of the code.
That's a good idea, for some internal documentation unrelated to the API.
It could, and the point to running it on a thread is to isolate the server, which is running on it's own tokio runtime, from calling into the application, as well as not enforcing an async style across the board. Protocol being a thread, which then calls into the application, makes it much harder for the application to block the server, even if the application doesn't use any threading.
It's meant as an example of how one could send the responder across threads and respond in an "asynchronous" way.
The public API is not concerned with sending data over a oneshot channel. It calls
The point is that with the current setup, using the if the application has to return a response, it makes the API fundamentally sync(although I guess an application could while blocking the protocol, still spawn some background work).
The point is instead to introduce a layer between the server and the application, which could enforce certain invariant about the request/response flow, while running independently of the server, and optionally also from the application. An example for such a use-case is found at #49
The goal of this PR is to provide a proof-of-concept for an API that:
If you know of a way to achieve these goals in a simpler way, I would be happy to read about it or see an alternative proof-of-concept. |
What I meant to say is that there's a lot going on under the hood that can't be configured, such as how
That makes a lot of sense. Putting the application logic on a seperate thread would keep the server from blocking, and that's a really good idea. But we don't need a message-passing struct to handle this: it would be simpler if we just had the server communicate directly with the Application over channels. Here's some psuedocode for how we could simplify this:
That way, there isn't any intermediate polling of responses: they get sent right back to the
I'm sorry for misunderstanding earlier the intents of this PR. However, I have some comments for each point:
|
In response to the numbered comments:
You mention handling of So, while an application might ultimately have to behave as a deterministic state machine, it could still consists of different components running in parallel, so the The unit test found here provides a good example of the ordering guarantees that can be provided, and tested, all the while retaining an asynchronous execution model involving parallel computation(in the case the main test thread and the protocol).
|
Alright, I see your rationale for this. While I am personally in favor of a future-based approach, I can see how this could be a worthy alternative. I also think it would be nice if there was some external interface for the |
Closing this one, I think the point of the example was made... |
Example of design proposed at #61 (comment)
Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/
)Added a relevant changelog entry:
clog add [section] [stanza] [message]
Reviewed
Files changed
in the github PR explorerBump Tendermint & ABCI version in README.md if version was updated
For Admin Use: